Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @TzahiTaub)

a discussion (no related file):
Waiting for @nimrod-starkware to fix a test, but can still be reviewed.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 45 at r1 (raw file):

    pub const ROOT: Self = Self(U256::ONE);

    #[allow(clippy::as_conversions)]

where is there an as here?

Code quote:

#[allow(clippy::as_conversions)]

crates/committer/src/patricia_merkle_tree/types.rs line 46 at r1 (raw file):

    #[allow(clippy::as_conversions)]
    pub const _FIRST_LEAF: Self = Self(U256::from_words(1_u128 << (Self::BITS - 1 - 128), 0));

so... from_words is const on U256, but lsh is not?
bummer

Code quote:

pub const _FIRST_LEAF: Self = Self(U256::from_words(1_u128 << (Self::BITS - 1 - 128), 0));

crates/committer/src/patricia_merkle_tree/types.rs line 46 at r1 (raw file):

    #[allow(clippy::as_conversions)]
    pub const _FIRST_LEAF: Self = Self(U256::from_words(1_u128 << (Self::BITS - 1 - 128), 0));

remove the underscore - if you want this to be private, remove the pub

Suggestion:

const FIRST_LEAF

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/compute_updated_skeleton_tree.rs line 51 at r1 (raw file):

        split_leaves(&self.tree_height, root_index, leaf_indices)
            .iter()
            .all(|x| !x.is_empty())

for clarity

Suggestion:

.all(|leaves_in_side| !leaves_in_side.is_empty())

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils.rs line 9 at r1 (raw file):

pub mod utils_test;

pub(crate) fn get_node_height(tree_height: &TreeHeight, index: &NodeIndex) -> TreeHeight {

docstring (the name is confusing)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils.rs line 43 at r1 (raw file):

            .last()
            .expect("leaf_indices unexpectedly empty.");
        assert_descendant(last_leaf);

redundant variable (non-blocking)

Suggestion:

        assert_descendant(leaf_indices
            .last()
            .expect("leaf_indices unexpectedly empty.")
        );

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils.rs line 46 at r1 (raw file):

    }

    let right_child_index = (*root_index << 1) + 1.into();

isn't this what was originally written?

Suggestion:

NodeIndex::ROOT

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils_test.rs line 13 at r1 (raw file):

    if jump == U256::ZERO {
        jump = (U256::MAX - start) / size_u256;
    }

this "feature" is not used, right?

Suggestion:

/// Creates an array of increasing random U256 numbers, with jumps of up to 'jump' between two
/// consecutive numbers.
fn create_increasing_random_array(size: usize, start: U256, jump: U256) -> Vec<U256> {
    let size_u256: U256 = size.try_into().unwrap();

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils_test.rs line 36 at r1 (raw file):

#[should_panic]
#[case::small_tree_not_descendant(
        3_u8, uint!("2"), &[uint!("8"), uint!("10"), uint!("14")], &[], &[])]

no auto-format in macros (like #[case] attributes)...
please try to conform to "correct" formatting manually

Suggestion:

#[case::small_tree_one_side(
    3_u8, U256::ONE, &[uint!("8"), uint!("10"), uint!("11")],
    &[uint!("8"), uint!("10"), uint!("11")], &[]
)]
#[case::small_tree_two_sides(
    3_u8, U256::ONE, &[uint!("8"), uint!("10"), uint!("14")],
    &[uint!("8"), uint!("10")], &[uint!("14")]
)]
#[should_panic]
#[case::small_tree_wrong_height(
    3_u8, U256::ONE, &[uint!("8"), uint!("10"), uint!("16")], &[], &[]
)]
#[should_panic]
#[case::small_tree_not_descendant(
    3_u8, uint!("2"), &[uint!("8"), uint!("10"), uint!("14")], &[], &[]
)]

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils_test.rs line 38 at r1 (raw file):

        3_u8, uint!("2"), &[uint!("8"), uint!("10"), uint!("14")], &[], &[])]

fn test_split_leaves(

remove newline

Suggestion:

        3_u8, uint!("2"), &[uint!("8"), uint!("10"), uint!("14")], &[], &[])]
fn test_split_leaves(

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils_test.rs line 58 at r1 (raw file):

#[rstest]

fn test_split_leaves_big_tree() {

remove newline

Suggestion:

#[rstest]
fn test_split_leaves_big_tree() {

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @TzahiTaub)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils.rs line 17 at r1 (raw file):

/// * The leaf indices array is sorted.
/// * All leaves are descendants of the root.
pub(crate) fn split_leaves<'a>(

this implementation is prettier and more concise - is it less efficient?
if not, please use it

Code quote:

pub(crate) fn split_leaves<'a>(

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 45 at r1 (raw file):

Oops, copy leftovers.

crates/committer/src/patricia_merkle_tree/types.rs line 46 at r1 (raw file):

Yes, that's what Yoni noticed.

crates/committer/src/patricia_merkle_tree/types.rs line 46 at r1 (raw file):

It's just for this PR as it's not used (test only). Will be used in the following. Prefer I bypass the warning?

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/compute_updated_skeleton_tree.rs line 51 at r1 (raw file):

Done.

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils.rs line 9 at r1 (raw file):

Is this OK?

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils.rs line 17 at r1 (raw file):

Do you mean the implementation that was in create_tree? It's still here (at the bottom). I just added assertions that the first and last leaves are actual descendants of the given root.

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils.rs line 46 at r1 (raw file):

I don't know, but IMO it makes sense to add 1 to the left son to get the right son and less sense to add ROOT to i.

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils_test.rs line 13 at r1 (raw file):

Yep, not ATM. Removed.

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils_test.rs line 36 at r1 (raw file):

Done.

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils_test.rs line 38 at r1 (raw file):

Done.

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils_test.rs line 58 at r1 (raw file):

Done.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 46 at r1 (raw file):

ah... ok I didn't realise the ocmpiler caught you. unblocking, either way is fine

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

a discussion (no related file):

PR that fixes the test

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

a discussion (no related file):

merged

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/utils.rs line 17 at r1 (raw file):

oh ok, removed block.
consider making assert_descendant a separate function

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

Reviewed 8 of 8 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)